-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New primitive to fill a line with as many doc parts as possible #1120
Conversation
ed785a8
to
9444106
Compare
Awesome! @rattrayalex could you take a look to see if that's what you had in mind? |
I've just pushed my latest prototyping, and written up a list of important things that still need to be worked on. It's taken me most of the day to get my head around Walder's paper and the Prettier code base, so I haven't had a chance to tackle the many edge cases I can see will occur with JSX and whitespace! |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is in fairly early stages, and I don't fully grok it yet, so I don't want to jump to any conclusions, but I'm a bit leery so far... seems fairly JSX-specific.
@vjeux @jlongster as a broader question, is flattening something we hope to bring to strings? It'll obviously involve AST-modification, but could be very nice.
If so, it might be worth starting there (since it'll be harder) or trying to build for both from the get-go.
src/doc-printer.js
Outdated
// console.log('only first fits', [first, second]); | ||
cmds.push(remainingCmd) | ||
if (typeof second !== 'string' || typeof first !== 'string') { | ||
cmds.push([ind, MODE_BREAK, "{' '}"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woah, this kind of JSX-specific stuff is definitely concerning... perhaps it could come out prior to release...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this felt strange but it was the easiest way to get something sane printing out!
Ideally the fill
implementation would be reasonably agnostic, but I don't know enough to make that work yet.
<span><Icon icon=\\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\" /> <Icon icon=\\"\\" /></span>; | ||
|
||
not_broken_end = | ||
"x = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious to see what the output is on the existing tests... you're welcome to add a new test fixture while you're developing to avoid running on multiple fixtures.
I know the work so far is pretty preliminary, but any chance you'd be willing to run w/ changes to existing tests too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've push up a new commit with all the original tests cases (and my single new test case), including all the change snapshots.
Lots has changed! It looks like most of the changes are due to collapsing newlines in JSX, but there are plenty of bugs as well 😛
src/printer.js
Outdated
// words.push(softline); | ||
// } | ||
|
||
value.replace(/^\s+|\s+$/g, "").split(/(\s+)/).forEach(word => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at this super in-depth, but it looks like the intent here would be to squash jsx+element mixtures to be as tight as possible?
So,
<div>
hello
world
</div>
would become
<div>
hello world
</div>
and
<div>
hello
<world />
</div>
would be come
<div>
hello<world />
</div>
?
(maybe you haven't thought this far ahead yet, which is fine – or maybe you plan to go back to the commented-out code soon enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have an intent at the moment, just playing around with what is possible 😀
Currently it is preserving new lines within JSX text, but I expect it may compact the join between text and tags.
I was hoping after a period of playing around we'd know enough about the possibilities to come up with a coherent set of layout rules that could then be implemented.
src/doc-printer.js
Outdated
cmds.push([ind, doc.break ? MODE_BREAK : mode, doc.contents]); | ||
|
||
break; | ||
case "fill": | ||
// include number of spaces | ||
cmds.push([ind, mode, " ".repeat(doc.parts.length - 1)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely confuses me, but I'm not super used to the printer.
src/doc-printer.js
Outdated
const firstCmd = [ind, MODE_FLAT, first]; | ||
const remainingCmd = [ind, MODE_BREAK, fill(remaining)]; | ||
|
||
const firstAndSecondCmd = [ind, MODE_FLAT, concat([first, second])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the firstAndSecond
gambit is primarily intended to understand how to split between text and elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The basic idea is copied from fill implementation in Wadler's paper (at the end of page 14).
I believe the idea is to check that the first two elements with both fit on a single line before joining them with a space.
If they don't fit on a single line then we need to join them with a new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, gotcha, interesting... thanks!
Thanks for taking a look @rattrayalex, very brave jumping in at this early stage 🙇🏻 I'm likely to be working on this in a very on/off fashion over the next few weeks, so don't feel in any hurry with giving feedback. High level approaches/direction stuff if definitely the most useful while I wrap my head around the algorithms/code. Also, I'm totally happy if this doesn't end up going anywhere. I'm already learning a lot about JSX and pretty printing 😀 |
cool, good luck & happy hacking! feel free to reach out, I'd be happy to help! |
It sounds neat, but my argument against it is that we never break strings by splitting one string into multiple (and converting into multiple strings with |
This PR looks really neat, thanks for the work! I don't have time to take a close look right now, but this would be great for JSX. |
5b2376a
to
ae09ade
Compare
I've managed to get this PR into a reviewable state! 😄 After much time spent wrapping my head around the many different ways of handling whitespace in JSX (so much respect for @rattrayalex having already tamed this!) I finally have a small and reasonably sane approach to filling a line with JSX children. The new I would be great if someone with a little more experience with the code could look at the changes I've made to |
ad4ffb5
to
e51b623
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks like really exciting progress! Seems like a generally correct algorithm with only a few minor issues/concerns from my end.
{" "} | ||
<span | ||
barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar | ||
/>{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this particular case looks like a regression to me... may not be a big deal.
foo | ||
</span> | ||
); | ||
|
||
after_break = ( | ||
<span> | ||
foo | ||
{" "} | ||
foo{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the existing behavior was intentionally added by @vjeux (IIRC, it used to behave as you have it for jsx whitespace)
@@ -146,13 +145,15 @@ nest_plz = ( | |||
|
|||
regression_not_transformed_1 = ( | |||
<span> | |||
{" "}<Icon icon="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" /> | |||
{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this surprising... should mirror the reverse cases, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(eg; similar_2 below)
{" "} | ||
long text long text | ||
long text long text long text long text long text long text long text long | ||
text <link>url</link> long text long text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🎉
@@ -179,6 +178,7 @@ leave_opening = ( | |||
submitLabel="Sign in with Google" | |||
> | |||
{" "} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regression
src/doc-printer.js
Outdated
break; | ||
} | ||
|
||
const split = parts[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the name split
confusing – is it assumed to always be a whitespace type (line
, softline
, etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to have a mirroring splitCmd
variable here, I found firstCmd
somewhat helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, I think I see... code like this might really clarify things for me as a reader...
const split = parts[1];
assert.ok(split.type === "line" || split.type === "...");
const splitFlatCmd = [ind, MODE_FLAT, split];
const splitBreakCmd = [ind, MODE_BREAK, split];
I think technically that's a bit less performant in some cases because not all scenarios require the instantiation of both the Flat and Break cmd's, but the overhead should be quite low.
I'm also not sure this will properly account for types like break-parent
, line-suffix
, line-suffix-boundary
, etc – which may not appear in the JSX context but are widespread elsewhere. Perhaps that's not worth building for at this point.
src/doc-printer.js
Outdated
break; | ||
} | ||
|
||
const second = parts[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second
seems confusing... it seems to be the third part?
src/doc-printer.js
Outdated
} | ||
|
||
const second = parts[2]; | ||
const remaining = parts.slice(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean parts.slice(3)
?
Thanks for the great feedback @rattrayalex! It might be a week or so before I get to work on this again, but when I do I'll aim at fixing up the regressions and edge cases. Your right that the fill algorithm could do with a bit of reworking, I was so happy I got it working I submitted the PR without refactoring it 😀 |
Awesome! I mean I think what you have is really great, and already very readable (all things considered) 😉 |
@rattrayalex I've pushed a hacked together quick fix that (I think) fixes all the regressions 😄 The code could probably do with a tidy up before merging, but I think the behaviour is looking good! |
src/printer.js
Outdated
@@ -3024,8 +3029,21 @@ function printJSXElement(path, options, print) { | |||
let forcedBreak = willBreak(openingLines); | |||
|
|||
const jsxWhitespace = options.singleQuote | |||
? ifBreak("{' '}", " ") | |||
: ifBreak('{" "}', " "); | |||
? ifBreak(concat([softline, "{' '}", softline]), " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mutations involved later on with this seem pretty bad... but I think you may be on the right track in general. How about something like this:
- change this to:
const jsxWhitespace = options.singleQuote ? "{' '}" : '{" "}';
- where
jsxWhitespace
is used above, change its usage toifBreak(concat([jsxWhitespace, softline]), " ")
(with the appropriate usage ofsoftline
, typically as indicated in the former code).
Thoughts?
@@ -122,8 +122,7 @@ long_open_long_children = ( | |||
colour="blue" | |||
size="large" | |||
submitLabel="Sign in with Google" | |||
/> | |||
d | |||
/>d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really seems wrong to me... is there any way to not use this algorithm if one of the elements has broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I write <div with="long attributes..." />blah
, the "blah" should really be on its own line if the div breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree!
In my first prototype it did have the behaviour of ensuring that the you always had a line break before and after elements that were broken. I must have lost that behaviour in one of my many reworkings of the code.
I'll aim to fix this up, hopefully some time next week.
I've pushed a few more commits that start to tidy up the code around the I've also fixed a couple of other bugs I found while adding new test cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming along... I'm still a bit concerned about how well this would translate to similar use-cases, like comments and strings.
Any thoughts?
src/printer.js
Outdated
if (i === 0) { | ||
groups.unshift(child); | ||
if (children.length === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a bit more clear if this were pulled out...
if (child === innerJsxWhitespace) {
if (children.length === 1) {
groups.unshift(solitaryJsxWhitespace);
groups.pop(); // remove unnecessary empty group
return;
} else if (i === 0) {
groups.unshift(leadingJsxWhitespace);
return;
} else if (i === children.length - 1) {
groups.push(trailingJsxWhitespace);
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now made this tweak!
src/printer.js
Outdated
@@ -3154,6 +3174,16 @@ function printJSXElement(path, options, print) { | |||
} | |||
}); | |||
|
|||
if (children[0] === innerJsxWhitespace) { | |||
children[0] = leadingJsxWhitespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like these mutations... why does it have to be done this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason it needs to be done this way, it was just the quickest way for me to test whether the idea of having different leading/trailing whitespace would work.
Very happy to try some different approaches to this. I wonder if the simplest would be to flatten the groups
array. That way we wouldn't need to duplicate the logic of converting inner whitespace to leading/trailing whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this, I believe this whole chunk of code can be removed.
It seems that children
is only used in the case where the JSX element is printed on a single line, in which case all the types of JSX whitespaces behave the same, so we don't need to special case leading/trailing/solitary whitespace.
I haven't been able to come up with a failing test case after removing this code, so it seems like a safe change to make.
@rattrayalex I haven't looked at this PR for a while, are you implying that we would rewrite comments and strings? I'm very hesitant to do that. Those are things people can format as they wish, and I think we should intentionally avoid touching them and allow people to do what they want. I feel like it would be too prescriptive/aggressive and too many edge cases to try to rewrite those. |
@vjeux I've fixed up the middle two bugs (words incorrectly output one per line, and extra blank lines. In the process of investigating how we decide to render the |
@karl Testing out your branch to see if I can get rid of some {' '}. Great work! I noticed one thing, not sure if it's intended: becomes (it adds the {' '}). However, this does not happen if aVariable is replaced by text. |
It sounds like |
Having the Do you think this is something that would block the PR? |
No, I'm happy to merge with this. Sorry I'm oncall this week and need to find time to run it through again. |
So so so sorry for the super long wait, I just ran this on our codebase and almost nothing changed, so I'm happy. Thanks for fixing all the edge cases :) |
For the issue with the comma, what about the following heuristic: if there is no space between the text and the jsx tag, then keep it that way? This is also an issue for - (<fbt:param name="Number of profile items to update">
+ (
+ <fbt:param name="Number of profile items to update">
{itemCount}
- </fbt:param>)
+ </fbt:param>
+ ) |
Thanks for finding the time to merge this @vjeux! And thanks to you and @rattrayalex for patiently taking me through my first major PR, it's been a pleasure 😀 |
☝🏻 I've opened a few issues to discuss possible follow on work from this PR. |
Awesome! |
This PR adds a new
fill
primitive for laying out documents. This will attempt to fill a line with as many parts of the document as will fit before moving to a new line.The idea for
fill
comes from page 14 of Wadler's paper: http://homepages.inf.ed.ac.uk/wadler/papers/prettier/prettier.pdfIf this works it could be a possible fix for #963.
I've aimed for as minimal a change as possible. We keep the existing JSX behaviour of respecting existing new lines. This means that the pretty printing is one way. Once we wrap text we won't ever unwrap it. This seemed like a sensible starting point!
Input:
Output: